-
Notifications
You must be signed in to change notification settings - Fork 10k
terraform test: refactor manifest file for simplicity #37412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: liamcervante/f-controlling-destroys
Are you sure you want to change the base?
terraform test: refactor manifest file for simplicity #37412
Conversation
…icorp/terraform into liamcervante/controlling-destroys/merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Liam - from a read-through of the diffs these changes look like they simplify things a lot. Could you please address the errors that are stopping the tests from running?
Error: ../../moduletest/graph/transform_state.go:114:23: t.StateManifest undefined (type *TestStateTransformer has no field or method StateManifest)
Error: ../../moduletest/graph/transform_state.go:126:4: t.EvalContext undefined (type *TestStateTransformer has no field or method EvalContext)
8fe1a5e
to
a460f37
Compare
Co-authored-by: Samsondeen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice improvements here. Thanks!
This PR aims to address the second of Sarah's concerns from the source branch. Namely, that the state files and manifest file are still be written even if they are empty / didn't error.
I did find the existing implementation of the manifest file to be confusing. States were being loaded and written at seemingly random times, and there was overlap between when things were being written into backends and into local files. There are two driving changes in this PR that I think simplify things a little bit:
This change did end up being larger than I expected, as I needed to move things around in quite a large number of places in order to get the kind of access needed to make the state loading work as intended. For that, I'm sorry. But, hopefully most of the smaller changes are just simple refactorings that you can understand.
There is also one behaviour change. When a run block with a backend fails to get destroyed, an entry is written into the manifest for that state file but it is no longer directly persisted to disk. Instead, the state as it exists with the failed destroy is written directly into the actual state backend. So, we have an entry in the manifest locally telling us the execution failed, but we don't keep duplicate copies of the state file around. Instead, during cleanup, the errored state is just loaded from the backend like normal instead of trying to sideload it locally. It seemed redundant and dangerous to hold competing copies of a state file in two locations.